New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ValueError being raised when plotting hist and hexbin on empty dataset (Fix #3886) #4119
Conversation
@@ -482,6 +482,9 @@ def test_hexbin_extent(): | |||
|
|||
ax.hexbin(x, y, extent=[.1, .3, .6, .7]) | |||
|
|||
def test_hexbin_empty(): | |||
# From #3886: creating hexbin from empty dataset raises ValueError | |||
ax.hexbin([], []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to create the Axes first. Also, I think you will want to use the @cleanup decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is now done via commit cb8539f.
Other than the testing issues Eric pointed out, this looks good. Could you also check the docstrings to make sure there are not any references left to this restriction? |
@@ -5607,7 +5606,7 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None, | |||
# basic input validation | |||
flat = np.ravel(x) | |||
if len(flat) == 0: | |||
raise ValueError("x must have at least one data point") | |||
return [], [], cbook.silent_list('No Patches') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want do draw no patches or patches of zero size? On consideration I think patches of zero size might be a better option here so client code which expects a non-zero length list does not have to check that it is true. For example if the user passes in bin edges, they should get back a list of zeros telling them that there are no counts in any of the bins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efiring @mdboom @WeatherGod @pelson Thoughts, I think that this is an important API choice that needs to be made.
I see that np.histogram returns an array of zeros and an array of boundaries when given an empty input array. I think we should follow that logic, which implies that we should be drawing zero-height (or width) patches. |
Yeah, I think that was one of my contributions to numpy... so, I certainly Have we fixed issues with autoscaling the axis when there are empty patches? Ben Root On Wed, Feb 18, 2015 at 2:31 PM, Eric Firing notifications@github.com
|
I have attempted to address the feedback (via 161eb17):
|
I think it should return the correct number of patches, drawn at the correct places. The application I am thinking of is if you doing something like vals, edges, patches = ax.hist([], bins=15, bottoms=range(N))
for v, p in zip(vals, patches):
f(v, p) Also, putting the patch at 0, 0 is probably not right thing to do because it can mess up auto-limiting. I strongly suspect that the right thing to do is to just let the results from the |
I've let the results of np.hist propagate through the code (after some minor adjustments to part of the code that needed non empty input) and set the patch to be zero size (via width of bar patch and linewidth of fill patch). Please have a look! |
@@ -5761,6 +5762,8 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None, | |||
for m, c in zip(n, color): | |||
if bottom is None: | |||
bottom = np.zeros(len(m), np.float) | |||
if input_empty: | |||
width = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @efiring 's comment: "I think we should follow that logic, which implies that we should be drawing zero-height (or width) patches."
Without setting width to 0, the patches for bar have size 0.1 and thus an empty graph (as desired) is not drawn.
Before (width 0.1, non empty graph):
However, currently the code still calculates the original width of 0.1 and then overwrites it, I will send a commit which moves it into a more appropriate place (where width, dw and boffset are originally calculated) in a future commit if this is the desired result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done via 40cf945
We probably do not need the pdf and svg tests in this case. Can you please add |
Done. |
…ng for hist. Fix ambiguous truth value in hexbin.
…step on empty input.
… hist and hexbin to use baseline images.
There were far too many useless commits in this PR (PEP8 fixes, comment/docstring changes, etc) given the relative size of the change (forgive my inexperience, this was my first PR). I've squashed some of the unnecessary commits together via rebase for a cleaner merge with master. Thanks! |
@jenshnielsen This is having strange issue with the docs, can you take a look? |
Looks like pip install of the doc dependencies just failed (the connection was cut in the middle of the IPython download). That really should make the build fail earlier. I have restarted the docs build |
|
||
# Massage 'x' for processing. | ||
# NOTE: Be sure any changes here is also done below to 'weights' | ||
if isinstance(x, np.ndarray) or not iterable(x[0]): | ||
if input_empty: | ||
x = np.array([[]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a 2D array instead of 1D?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind, I see why.
…nd not overwrite weights on empty input.
ENH : Allow empty input to hist and hexbin closes #3886
@umairidris Thank you! |
Fix #3886